Clean Code

Кирилл Корняков, Андрей Морозов

Сентябрь 2015

Содержание

Зачем?

Наши клиенты не смотрят на исходный код.
Почему мы должны держать его в чистоте?

Генеральный директор компании по разработке ПО,
Нижний Новгород, Август 2010

Преимущества чистого кода

Откуда берется плохой код?


Кто же виноват?

Как результат...

Содержание

Что такое качество кода?

Признаки качества кода

Хорошо

Плохо

  • Простой, логичный, понятный
  • Чистый, структурированный
  • Краткий, лаконичный
  • Единообразный (стилистически единый)
  • Непонятный, заумный, сложный
  • Запутанный, шумный, визуально нагруженный
  • Длинный, повторяющийся
  • Беспорядочный, пестрый

Ключевые понятия

API — самое важное

Matrix createMatrix(int a1, int a2);
Matrix createMatrix(int width, int height);
List<PECustomerDetailsData> retrieveValidateAndConvertCustomerSpecificDataIntoPresentationEntities();
customer.customerName = "Bill";
  public void do(); // up

  private int i1;   // down

Содержание

Важность именования

Программист не стал выдумывать и назвал детей
новый сын(1) и новый сын(2)

Небрежное именование

Что делает следующий фрагмент?

public List<int[]> getThem() {
  List<int[]> list1 = new ArrayList<int[]>();
  for (int[] x: theList) {
    if (x[0] == 4)
      list1.add(x);
  }
  return list1;
}

Осмысленные имена

А теперь?

public List<int[]> getFlaggedCells() {
  List<int[]> flaggedCells = new ArrayList<int[]>();
  for (int[] cell: gameBoard) {
    if (cell[STATUS_VALUE] == FLAGGED)
      flaggedCells.add(cell);
  }
  return flaggedCells;
}

Следим за уровнем абстракции

public List<Cell> getFlaggedCells() {
  List<Cell> flaggedCells = new ArrayList<Cell>();

  for (Cell cell: gameBoard) {
    if (cell.isFlagged()) {
      flaggedCells.add(cell);
    }
  }

  return flaggedCells;
}

Сравним...

public List<int[]> getThem() {
  List<int[]> list1 = new ArrayList<int[]>();
  for (int[] x: theList) {
    if (x[0] == 4)
      list1.add(x);
  }
  return list1;
}
public List<Cell> getFlaggedCells() {
  List<Cell> flaggedCells = new ArrayList<Cell>();

  for (Cell cell: gameBoard) {
    if (cell.isFlagged()) {
      flaggedCells.add(cell);
    }
  }

  return flaggedCells;
}

Magic numbers

    int dailyPay = hourlyRate * 8;
    double milesWalked = feetWalked / 5280;
    int step = width * 4;

Не используйте их!

Вместо этого используйте:

Шум!

public class Part {
  private String m_dsc;
  void setName(String name) {
    m_dsc = name;
  }
}




public class Part {
  private String description;
  void setDescription(String description) {
    this.description = description;
  }
}

Одно слово для каждой концепции

Имена классов

Имена классов и объектов должны представлять собой
существительные и их комбинации.

Хорошо

Плохо

Имена методов

Имена методов представляют собой
глаголы или глагольные словосочетания.

String name = employee.getName();
customer.setName("Mike");
if (paycheck.isPosted()) ...

Именование переменных и членов класса

Хорошо

public class IncompleteOrder {}
public int currentPosition = -1;
const int WORK_HOURS_PER_DAY = 8;
private bool isBlocked // can, is, has


Плохо

public class incompleteOrder {}
private bool flag;

public const int NUMBEROFCONTEXTS = 10;
private int collectionsize;
private string m_strName;
private byte _array;

Итого. Именование

Советы

Содержание

Функции

Какова нормальная длина функции?

In a completely uncommented 2000 line method:

    {
      {
        while (..) {
          if (..){
          }

          ... (just putting in the control flow here, imagine another few hundred ifs)
          if(..) {
                if(..) {
                    if(..) {
                    ...
                    (another few hundred brackets)
                    }
                }
          } //endif
          ...

Оптимальное количество аргументов

    int OverlayFlatVideos(int numberOfFlatVideos,
                          int currentFrameIdx,
                          OverlayAllVideosParams^ previewParams,
                          std::vector<bool>& StreamProcessed,
                          std::vector<acvCapture*>& flatVideoReaders,
                          std::vector<double>& fpsFlatVideos,
                          bool sharedReflection,
                          std::vector<CvMat*>& reflectionsFlatToDome,
                          CvSize& fullDomeSize,
                          std::vector<IplImage*>& masks,
                          std::vector<IplImage*>& borderSmoothImage,
                          CvSize& maskSize,
                          IplImage*& imageFullDome,
                          CvMat*& tempRef,
                          double fps,
                          int& numberOfVideoReaders,
                          IplImage*& imageReflected,
                          IplImage*& imageFullDomeCopy,
                          InterpolationMethod inMethod) // 19

WinApi C++

        hThreadArray[i] = CreateThread(
            NULL,                   // default security attributes
            0,                      // use default stack size
            MyThreadFunction,       // thread function name
            pDataArray[i],          // argument to thread function
            0,                      // use default creation flags
            &dwThreadIdArray[i]);   // returns the thread identifier




    wnd.CreateWnd(hInstance, wcname, NULL, WS_VISIBLE|WS_OVERLAPPEDWINDOW,
        NWin::SRect(NWin::SPoint(CW_USEDEFAULT,CW_USEDEFAULT),
        NWin::SSize(600,400)), NULL,
        LoadMenu(hInstance, resWapp), NULL);

Выходные параметры функции

    static void GetSupportDocFilePath(out string supportDocFilePath) {
        supportDocFilePath = new ConfigurationHelper().SupportFilePath;
    }

    static string GetSupportDocFilePath() {
        return new ConfigurationHelper().SupportFilePath;
    }

Убийственная сложность

float _______ ( float number )
{
  long i;
  float x2, y;
  const float threehalfs = 1.5F;

  x2 = number * 0.5F;
  y  = number;
  i  = * ( long * ) &y;                       // evil floating point bit level hacking
  i  = 0x5f3759df - ( i >> 1 );               // what the fuck?
  y  = * ( float * ) &i;
  y  = y * ( threehalfs - ( x2 * y * y ) );   // 1st iteration
//      y  = y * ( threehalfs - ( x2 * y * y ) );   // 2nd iteration, this can be removed
  return y;
}

Условия

    if (splitParameters->projectorVideos == nullptr ||
        System::String::IsNullOrEmpty(splitParameters->splitSettings) ||
        splitParameters->projectorWidth <= 0 ||
        splitParameters->projectorHeight <= 0)

    if (timer.HasExpired() && !timer.IsRecurrent())
    if (ShouldBeDeleted(timer)) {}

    if(isValid == false) {}
    if(!canEditPrice) {}

Предпочитайте исключения кодам ошибок

    public void SendShutDown() {
        var handle = GetHandle(device);
        if (handle != DeviceHandle.INVALID) {
            var err = OpenDevice(handle);
            if (err == NULL) {
              //
            }
            else {
              Logger.Log("Can't open device. Error: " + err.ToSting());
            }
        }
        else {
            Logger.Log("Invalid handle for: " +
            device.ToSting());
        }
    }

С использованием исключений

    public void TrySendShutDown() {
        try {
          SendShutDown();
        }
        catch() {
        // ...
      }
    }

    public void SendShutDown() {
      var handle = GetHandle(device);
      OpenDevice(handle);
      //...
    }

Итого. Функции

Содержание

Комментарии

    // When I wrote this, only God and I understood what I was doing
    // Now, God only knows

    ...

    // Magic. Do not touch.

    ...

    // sometimes I believe compiler ignores all my comments

    ...

    // I'm sorry.

    ...

    // I am not sure if we need this, but too scared to delete.
catch (Exception e) {
    //who cares?
}

Комментарии

Они полезны?

Большие заголовки

    /*---------------------------------------------------------------
    -----------------------
    Created by: NANDA
    Created Date: 01-AUG-2009
    Modified by:
    Procedure Description: Fetches menu items based on the given
    user permission
    Input Parameters: LoginEntry user
    Output Parameters: none
    ----------------------------------------------------------------
    --------------------*/
    public BEMenuList FetchMenuItems(LoginEntity user) {
    ...
    }

Устаревший комментарий

    ...
    // Gets the login user id
    // Gets the CRM details
    FetchCrmDetails();
    ...

Избыточный комментарий

    // If the server variable is empty, throw the error message
    if (loginUserId == null)
    {
        throw new Exception("No User Id");
    }

Запутывающий комментарий

    public void LoadProperties() {
        try
        {
            var propertiesPath = propertiesLocation +
            "/" + PROPERTIES_FILE;
            var propertiesStream = File.Open(propertiesPath);
            loadedProperties.Load(propertiesStream);
        }
        catch (IOException ex) {
            //If file with properties doesn’t exist,
            //default settings are loaded
        }
    }

Закоментированный код

//#region ShowHyperLink
///// <summary>
///// Show the hyperlink controls
///// </summary>
//private void ShowHyperLink()
//{
//  asphypCreateAccounts.Visible = true;
//  asphypCreateAccounts.NavigateUrl = "CreateAccount.aspx";
//  asphypCreateAccounts.Text = WebConstants.CreateAccountHyperLink;
//  asphypCreateExtUser.Visible = true;
//  asphypCreateExtUser.NavigateUrl = "ManageExternalUsers.aspx";
//  asphypCreateExtUser.Text = WebConstants.ExternalUserHyperLink;
//}
//#endregion

Еще хуже

asplblAcceptDeclineDate.Text = contractHistoryList[0].AcceptedDate.ToString();
if (contractHistoryList[0].IsMultiple)
{
    asplblMultiplePublish.Text = "Yes";
}
else
{
    asplblMultiplePublish.Text = "No";
}
//if (contractHistoryList[0].IsLegalApprovalRequierd == true)
//{
//  asplblLegalApproval.Text = "Yes";
//}
//else
//{
//  asplblLegalApproval.Text = "No";
//}
asphdnFileGuid.Value = contractHistoryList[0].FileGuid.ToString();
//int contractRefNo = Convert.ToInt32(asphdnFileMasterld.Value.ToString());
//Session(WebConstants.CONTRACT_REF_NO_SESSION_KEY] = contractRefNo;
asptxtReasonForRejection.Text = string.Empty;

Дезинформация

/**
 * Always returns true.
 */
public boolean isAvailable()
{
    return false;
}

Позволительные комментарии

// format matched kk:mm:ss EEE, MMM dd, yyyy
Pattern timeMatcher = Pattern.Compile("\\d*:\\d*:\\d* \\w*, \\w* \\d*, \\d*");
    //TODO: ...
    //FIXME: ...
    //HACK, NOTE, WARNING

Итого. Комментарии

"Don’t comment bad code — rewrite it!"

B. Kernighan, P. Plauger "The Elements of Programming Style"

Содержание

Форматирование

http://www.ioccc.org/2011/akari/hint.html

Горизонтальное выравнивание

Плохо

customer.CalculateCredit ( fromDate );
customer.CalculateCredit(fromDate , toDate);
if ( IsValid ) i ++

Хорошо

if(customer.IsValid && customer.Credit == 0.0)
position = new Location(position.x + 10, position.y);
return IsValid ? cdd : DateTime.MaxDate;

Вертикальное выравнивание

using MVCS.Diff4Big.Domain.ImageEntities;
using MVCS.Diff4Big.Domain.Specifications;
namespace MVCS.Diff4Big.Domain.Comparison.FT {
    public class ByteByByte : ITileComparator {
        private readonly LengthBased lengthBased = new LengthBased();
        public IDeltaContainer Compare(IContentTile tile1, IContentTile tile2, ChangeType changeType) {
            var bytesRangeEqualsSpecification = new BytesRangeEquals(tile1.Content);
            var baseResult = lengthBased.Compare(tile1, tile2, changeType);
            if (baseResult != null) return baseResult;
            if (bytesRangeEqualsSpecification.IsSatisfiedBy(tile2.Content)) return null;
            if (changeType == ChangeType.ImageThematicalTile) {
                return new ThematicalTileContainer((ThematicalBig1Tile) tile2.Clone());}
            return new TileContainer((IContentTile) tile2.Clone(), changeType);
        }
    }
}

Вертикальное выравнивание

using MVCS.Diff4Big.Domain.ImageEntities;
using MVCS.Diff4Big.Domain.Specifications;

namespace MVCS.Diff4Big.Domain.Comparison.FT {

    public class ByteByByte : ITileComparator {
        private readonly LengthBased lengthBased = new LengthBased();

        public IDeltaContainer Compare(IContentTile tile1, IContentTile tile2, ChangeType changeType) {
            var bytesRangeEqualsSpecification = new BytesRangeEquals(tile1.Content);

            var baseResult = lengthBased.Compare(tile1, tile2, changeType);
            if (baseResult != null) return baseResult;

            if (bytesRangeEqualsSpecification.IsSatisfiedBy(tile2.Content)) return null;

            if (changeType == ChangeType.ImageThematicalTile) {
                return new ThematicalTileContainer((ThematicalBig1Tile) tile2.Clone());
            }
            return new TileContainer((IContentTile) tile2.Clone(), changeType);
        }
    }
}

Рекомендация

  if (a == 0);
    a++;

  while(a++ != magic_number);
    a = a << 2;



  if (a == 0) {
    a++;
  }

  while(a++ != magic_number) {}
    a = a << 2;

Содержание

Ключевые принципы (фольклор)

Ключевые моменты

Заключение: Правило бойскаута

Всегда оставляй лагерь чище, чем ты его нашел.

Контрольные вопросы

  1. Признаки хорошего и плохого кода
  2. Ключевые понятия при разговоре о чистоте кода
  3. Рекомендации по
    • Именованию
    • Оформлению функций
  4. Комментарии и чистый код
  5. Ключевые принципы

Спасибо за внимание!

Вопросы?